-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Apply radiative boundary conditions to Z4c variables #19
Conversation
@@ -12,8 +12,6 @@ BOOLEAN calc_constraints "Calculate constraints" STEERABLE=recover | |||
{ | |||
} yes | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you deleting these lines? They separate two groups of parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -13,7 +13,7 @@ USES INCLUDE HEADER: simd.hxx | |||
USES INCLUDE HEADER: sum.hxx | |||
USES INCLUDE HEADER: vec.hxx | |||
USES INCLUDE HEADER: vect.hxx | |||
|
|||
USES INCLUDE HEADER: newradx.hxx | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add another empty line here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. However, the following comment suggests arranging the header files alphabetically. For now, I have added an empty line. Should I rather arrange everything alphabetically and remove the empty line?
Z4c/src/apply_newrad.cxx
Outdated
#include "loop_device.hxx" | ||
#include "newradx.hxx" | ||
|
||
using namespace Loop; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not using Loop
nor std
in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -13,7 +13,7 @@ USES INCLUDE HEADER: simd.hxx | |||
USES INCLUDE HEADER: sum.hxx | |||
USES INCLUDE HEADER: vec.hxx | |||
USES INCLUDE HEADER: vect.hxx | |||
|
|||
USES INCLUDE HEADER: newradx.hxx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list of include files should be sorted alphabetically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above comment suggested instead to add another line instead. Let me know if you prefer an additional line or all header files sorted alphabetically.
@jaykalinani Can you also add a test case for your changes? Remember that test cases should be small and fast. We don't need to do a convergence test – just a quick setup with a small grid running for 3 iterations would suffice. |
Added a test case based on |
Apply radiative boundary conditions to Z4c variables using the new NewRadX thorn.